-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Transform robot pose lifecycle #793
Transform robot pose lifecycle #793
Conversation
Not blocking: but why incur the overhead of the service call when you could have the |
There were several dependencies to getting the pose that costmap already had, but navfn didn't.
I especially didn't want to have to add two new parameters to navfn to specify the transform tolerance and robot base frame because they'd be a duplicate of what's already on costmap. Who knows what would go wrong when they inevitably go out of sync. I don't particularly like this solution, but I don't have a better one at the moment. I was going to not submit this change until I had a better one, but we need something to allow navfn to get the pose via the transform to avoid having to add a different workaround in AMCL to constantly publish the amcl_pose. (There are startup race conditions otherwise) |
On the robot frame - assuming |
Agreed. I had meant to do some measurements on latency before continuing with this approach, but I haven't had the time to do it yet. I was hoping that we could compose the world model, the global planner and local planner and get super low latency, while still separating concerns. Navfn is already getting the costmap over a service call. If service calls are fast enough, then getting the pose over a service call should be OK as well; we can always execute the calls in parallel to cut latency even further. On the other hand, if service calls are too slow, we're already broken because of the costmap call, and this change will help to highlight that problem. I could put all the logic into nav2_util or some such place, but I felt this was a bit cleaner, and hasn't been shown to be a problem yet. I figured this was the lesser of two evils. How would you prefer to do it? |
I'm also of the opinion that the service calls for costmaps probably aren't the best thing unless the goal of it is to only run the costmap when you need data from it and stop the background updates. Otherwise if you're publishing costmaps at N hz, just subscribing to use the latest set of data is preferable. If you want on demand costmaps, I would also think its better to have in the same process (which composible would make it so, but force it that way by having the model as an object the planner has direct access to). If the latency isn't substantial though, then maybe its fine, however just populating a message of a 100m x 100m map to hand over to the planner is some computational time over just querying the hosted object for some particular set of information. Back to the question: I think its a super common thing for people to be having some thread spinning at lets say 20-50hz where a component of an operation is requesting the pose. If you have a couple things going on, that's 100 requests a second from the costmap thread to give you a pose, versus having each process use TF directly. Having it as a util seems more portable and clean to me so there's not a bottle neck when down the line we have 3-6 threads asking for the pose at the same time. Realistically it would be fabulous to have alot of these threads share a TF buffer but we would have to create our own |
5b3d02f
to
b29f63d
Compare
b29f63d
to
43c94bd
Compare
Codecov Report
@@ Coverage Diff @@
## master #793 +/- ##
==========================================
- Coverage 21.12% 20.96% -0.16%
==========================================
Files 184 184
Lines 9448 9447 -1
Branches 2317 2312 -5
==========================================
- Hits 1996 1981 -15
- Misses 6311 6326 +15
+ Partials 1141 1140 -1
Continue to review full report at Codecov.
|
nav2_msgs/srv/GetCostmap.srv
Outdated
@@ -4,3 +4,5 @@ | |||
nav2_msgs/CostmapMetaData specs | |||
--- | |||
nav2_msgs/Costmap map | |||
geometry_msgs/PoseStamped pose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you are using the PoseStamped msg in this srv in the code and you are already creating a GetRobotPose srv, so I think these additional fields should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Thanks.
const std::string layer = "master"); | ||
|
||
// get latest robot pose from the world model | ||
geometry_msgs::msg::Pose getRobotPose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is getRobotPose
being called here in the header like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a function prototype here, not a call.
I'd like to get this PR moving because I believe it will be very useful for collision checking in #737 (I want the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to rebase and resolve conflicts
Longer time intervals can't be converted to smaller intervals.
706d79a
to
efb8302
Compare
Basic Info
Description of contribution in a few bullet points
Further work that may be required